feat(connector):Add ironrdp-vmconnector crate, integrated into existing ironrdp-client and ironrdp async #841
Conversation
|
@irvingoujAtDevolution I don’t remember very well. What is different from the previous version? |
|
@CBenoit |
| pub async fn run_until_handover( | ||
| credssp_finished: &mut CredSSPFinished, | ||
| framed: &mut Framed<impl FramedRead + FramedWrite>, | ||
| mut connector: VmClientConnector, | ||
| ) -> ConnectorResult<ClientConnector> { | ||
| let result = loop { | ||
| single_sequence_step(framed, &mut connector, &mut credssp_finished.write_buf).await?; | ||
|
|
||
| if connector.should_hand_over() { | ||
| break connector.hand_over(); | ||
| } | ||
| }; | ||
|
|
||
| info!("Handover to client connector"); | ||
| credssp_finished.write_buf.clear(); |
There was a problem hiding this comment.
question: Is handover a terminology used by VMConnect protocol?
There was a problem hiding this comment.
Not really, it's only make sense in this context
CBenoit
left a comment
CBenoit
left a comment
There was a problem hiding this comment.
question: What about the extra option EnhancedMode? I think Hyper-V understands the following format now: <GUID>;EnhancedMode=1
|
We were having CredSSP issues on enhanced mode, I intend to have that support in separate pull request. |
Coverage Report 🤖 ⚙️Past: New: Diff: -0.64% [this comment will be updated automatically] |
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
2 times, most recently
from
e28091e to
349b21f
Compare
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
from
082acb5 to
69264ff
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for HyperV VmConnect connections to the IronRDP client. The changes enable clients to connect to HyperV virtual machines using a VM-specific connector that handles the specialized protocol requirements.
- Introduces a new
ironrdp-vmconnectcrate to handle HyperV VM-specific connection logic with NTLM-only CredSSP - Refactors connector architecture to use trait-based approach (
ConnectorCore,SecurityConnector,CredsspSequenceFactory) for polymorphism - Updates RDCleanPath protocol to support optional x224 connection PDUs for VM connections
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web-client/iron-svelte-client/src/lib/login/login.svelte | Adds UI field for HyperV VmConnect ID input |
| web-client/iron-remote-desktop-rdp/src/main.ts | Exports new vmConnect extension function |
| crates/ironrdp-vmconnect/src/lib.rs | New module exposing VM connector functionality |
| crates/ironrdp-vmconnect/src/connector.rs | Implements VmClientConnector with VM-specific state machine |
| crates/ironrdp-vmconnect/src/credssp.rs | Implements NTLM-only CredSSP sequence for VM connections |
| crates/ironrdp-vmconnect/src/config.rs | Defines VM-specific credentials and configuration types |
| crates/ironrdp-web/src/session.rs | Integrates VM connector with web client connection flow |
| crates/ironrdp-client/src/rdp.rs | Updates TCP/WebSocket connection logic to support VM connectors |
| crates/ironrdp-client/src/config.rs | Adds CLI arguments and config for VM connections |
| crates/ironrdp-rdcleanpath/src/lib.rs | Changes x224 PDU fields to Option<T> for VM support |
| crates/ironrdp-connector/src/lib.rs | Introduces trait-based connector abstraction |
| crates/ironrdp-connector/src/credssp.rs | Refactors CredSSP into trait for extensibility |
| crates/ironrdp-connector/src/connection.rs | Implements new traits for ClientConnector |
| crates/ironrdp-async/src/connector.rs | Updates async connector to work with trait objects |
| crates/ironrdp-async/src/vmconnector.rs | Adds VM connector lifecycle management functions |
| crates/ironrdp-blocking/src/connector.rs | Updates blocking connector for trait objects |
| crates/ironrdp-testsuite-/tests/.rs | Updates tests to match new async API signatures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@irvingoujAtDevolution Can you rebase on top of master and address Copilot’s suggestions? |
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
from
9996aa4 to
08e16f8
Compare
|
@irvingoujAtDevolution Can you double check with |
|
@irvingoujAtDevolution Can you resolve the conflicts with master? |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 46 out of 49 changed files in this pull request and generated 3 comments.
Files not reviewed (2)
- web-client/iron-remote-desktop-rdp/package-lock.json: Language not supported
- web-client/iron-remote-desktop/package-lock.json: Language not supported
Comments suppressed due to low confidence (6)
web-client/iron-remote-desktop/src/main.ts:1
- Remove debug console.log statement with inappropriate language before merging to production.
web-client/iron-remote-desktop/src/iron-remote-desktop.svelte:1 - Corrected spelling of 'Cnava' to 'Canvas'.
crates/ironrdp-web/src/clipboard.rs:1 - Method call
is_empty()should beitems().is_empty()to match the implementation. TheClipboardDatatype doesn't have anis_empty()method directly, only through its items.
//! This module implements browser-based clipboard backend for CLIPRDR SVC
web-client/iron-remote-desktop-rdp/public/package.json:1
- Version appears to be downgraded from 0.6.1 to 0.4.2. This is likely unintentional and should be corrected to maintain proper semantic versioning.
crates/ironrdp-web/src/session.rs:1 - [nitpick] TODO comment references 'nullptr' which is C++ terminology. In Rust, use 'None' or 'null' when referring to Option types. Update comment to use appropriate Rust terminology.
crates/ironrdp-web/src/session.rs:1 - [nitpick] TODO comment references 'nullptr' which is C++ terminology. In Rust, use 'None' when referring to Option types. Update comment to use appropriate Rust terminology.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [patch.crates-io] | ||
| # FIXME: We need to catch up with Diplomat upstream again, but this is a significant amount of work. | ||
| # In the meantime, we use this forked version which fixes an undefined behavior in the code expanded by the bridge macro. | ||
| diplomat = { git = "https://github.com/CBenoit/diplomat", rev = "6dc806e80162b6b39509a04a2835744236cd2396" } |
There was a problem hiding this comment.
Using a Git revision dependency instead of a published version. Ensure this dependency is temporary and document why this specific revision is required, or publish a new version of sspi-rs to crates.io.
| diplomat = { git = "https://github.com/CBenoit/diplomat", rev = "6dc806e80162b6b39509a04a2835744236cd2396" } | |
| diplomat = { git = "https://github.com/CBenoit/diplomat", rev = "6dc806e80162b6b39509a04a2835744236cd2396" } | |
| # FIXME: We are using a specific commit of sspi-rs because the required bugfix/feature is not yet published to crates.io. | |
| # Once https://github.com/Devolutions/sspi-rs releases a new version including commit 370951c1b017bfef4276185b374345e8b6b1e532, | |
| # this should be updated to use the published version from crates.io. |
There was a problem hiding this comment.
@irvingoujAtDevolution Most likely, you can remove the git patch here.
There was a problem hiding this comment.
I'll do this is separate PR
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
from
de97850 to
95877a6
Compare
irvingoujAtDevolution
force-pushed
the
vmconnect-use-vmconnector
branch
from
95877a6 to
4acff79
Compare
Co-authored-by: Copilot <[email protected]>
|
@CBenoit Ready for merge! |
Hello @CBenoit , I added dynamically dispatched trait for hyperv connector and normal connectors. The main drive is to avoid copy and paste in previous version. I did it this is way is also to reuse existing code as much as possible, instead of copy each and every method and add a _vmconnect suffix. This might not be the best solution, let me know your thoughts.